Skip to content

Fix prefix_same_as_start iterator correctness gaps (#15606) - #15606

Open
xingbowang wants to merge 1 commit into
meta:mainfrom
xingbowang:export-D129901533
Open

xingbowang wants to merge 1 commit into
meta:mainfrom
xingbowang:export-D129901533

Conversation

@xingbowang

@xingbowang xingbowang commented Nov 6, 2026 •

Copy link
Copy Markdown
Contributor

Summary:

Why the change is required:
The crash test found prefix_same_as_start iterators returning wrong keys in
two ways, both caused by child iterators using prefix filters (SST prefix
filter, memtable prefix bloom, LevelIterator prefix exhaustion) for a key in a
different prefix than the one the caller iterates:

  1. A scan with no seek prefix to bound it: SeekToFirst() / SeekToLast() (with
    a bound they take the prefix from the bound), or Seek() / SeekForPrev()
    with a target outside the prefix extractor's domain. With a range
    tombstone that crosses a prefix boundary, MergingIterator reseeks older
    children to the tombstone boundary key, a prefix filter can drop a child,
    and the scan can skip live keys. Read-path tombstone conversion
    (min_tombstones_for_range_conversion, EXPERIMENTAL) trusts these scans, so
    a skipped live key could be covered by a converted tombstone and deleted
    for good. The same holds for implicit prefix seek (default ReadOptions).
    Also, SeekToLast() with iterate_upper_bound becomes
    SeekForPrev(upper_bound), which takes its prefix from an exclusive bound
    the iterator can never return: keys "abcd1".."abcd9" with upper bound
    "abce" return nothing.
  2. With prefix_same_as_start and total_order_seek both set, a direction change
    (Next() after Prev()) can skip keys inside the seek prefix. Under
    DBOptions::prefix_seek_opt_in_only, total_order_seek is forced on, so every
    prefix_same_as_start iterator is exposed.

Previous behavior:

  1. SeekToFirst()/SeekToLast() and out-of-domain seeks in prefix seek mode were
    never promised to match total order (the option docs call keys outside the
    seek prefix undefined, and the Prefix-Seek wiki says these calls are not
    well supported), but they ran, could return wrong results, and could
    create read-path range tombstones.
  2. Since prefix_seek_opt_in_only was added (9.7.0), prefix_same_as_start and
    auto_prefix_mode enable prefix filtering even with total_order_seek=true,
    so for iterators total_order_seek only turns off the implicit prefix seek
    of default ReadOptions. This was stated in that release note, but not in
    options.h, where total_order_seek still read as "always total order".
    DBIter still treated total_order_seek=true as "the inner iterator is total
    order". So DBIter::ReverseToForward() skipped its reseek to the current
    key, and MergingIterator::SwitchToForward() reseeked the other children to
    the previous key, which can have another prefix. A child without that
    prefix was dropped by its prefix filter, losing its keys in the seek
    prefix.

New behavior:

  1. With DBOptions::prefix_seek_opt_in_only, a prefix_same_as_start scan must
    take its prefix from an in-domain seek key. SeekToFirst() and SeekToLast(),
    with or without bounds, leave the iterator !Valid() with
    Status::NotSupported, and Seek()/SeekForPrev() with an out-of-domain target
    leave it !Valid() with Status::InvalidArgument. Callers Seek() or
    SeekForPrev() to a key in the wanted prefix instead.
    Without prefix_seek_opt_in_only, iteration results are unchanged, but an
    iterator in prefix seek mode (prefix_same_as_start or implicit prefix seek)
    no longer creates read-path range tombstones from a scan started by
    SeekToFirst() without iterate_lower_bound, SeekToLast() without
    iterate_upper_bound, or an out-of-domain seek target.
  2. In DBIter, prefix_same_as_start takes precedence over total_order_seek and
    auto_prefix_mode, so DBIter reseeks on a direction change. A
    prefix_same_as_start iterator returns the same keys with or without
    total_order_seek, and so with or without prefix_seek_opt_in_only for
    Seek()-started scans.
    Total order and auto_prefix_mode iteration results are not affected.

Implementation:

  • GetIteratorPrefixMode() in options_helper.h decides how an iterator uses the
    prefix extractor (kTotalOrder, kAuto, kPrefix), with the order
    prefix_same_as_start > auto_prefix_mode > total_order_seek > implicit prefix
    seek. DBIter uses it for its total-order assumption; that is the
    direction-change fix. MemTable, PlainTable, the block-based table
    check_filter, LevelIterator files_filterable_on_seek_ and db_stress
    VerifyIterator use it where it gives the same result as before. The
    MergingIterator prefix flag, LevelIterator prefix exhaustion, the SST
    auto_prefix_mode upper bound check and the hash index keep their old
    conditions, so their behavior is unchanged.
  • DBIter: under prefix_seek_opt_in_only, prefix_same_as_start SeekToFirst()
    and SeekToLast() return NotSupported before redirecting to a bound, and a
    new CheckSeekTargetPrefix() at the start of Seek()/SeekForPrev() returns
    InvalidArgument for an out-of-domain target. Otherwise DBIter remembers when
    the current scan uses prefix seek but has no prefix to bound it (unbounded
    SeekToFirst()/SeekToLast(), or out-of-domain target), and
    MaybeInsertRangeTombstone(), the only place that writes a converted
    tombstone, does nothing then.
  • options.h: ReadOptions::total_order_seek now starts with its iterator
    meaning (turns off the implicit prefix seek; prefix_same_as_start and
    auto_prefix_mode take precedence; forced on by prefix_seek_opt_in_only).
    auto_prefix_mode notes that prefix_same_as_start takes precedence.
    prefix_seek_opt_in_only and prefix_same_as_start document the NotSupported /
    InvalidArgument cases and the scans that are not well supported.
    iterate_lower_bound allows a bound with another prefix under
    prefix_same_as_start.
  • Release notes: a behavior change for NotSupported / InvalidArgument and the
    conversion change, and a bug fix for the direction change.
  • db_stress: new prefix_seek_opt_in_only flag, randomized by db_crashtest.py.
    TestIterate sometimes sets total_order_seek together with
    prefix_same_as_start, and under prefix_seek_opt_in_only checks that
    SeekToFirst()/SeekToLast() return NotSupported. The prefix_same_as_start
    SeekToFirst() verification is removed: it checks a known limitation, and
    range tombstones left by earlier crash-test runs would make it fail. The
    bug it covered keeps its unit tests in file/prefetch_test.cc.

Safety, compatibility, and limitations:

  • No API or format change. Users with prefix_seek_opt_in_only who call
    SeekToFirst()/SeekToLast() on a prefix_same_as_start iterator, or seek it to
    an out-of-domain key, now get NotSupported / InvalidArgument; they can
    Seek()/SeekForPrev() to a key in the wanted prefix, or use total order or
    auto_prefix_mode.
  • prefix_same_as_start with total_order_seek or prefix_seek_opt_in_only now
    reseeks on a direction change, the cost plain prefix_same_as_start already
    pays.
  • Without prefix_seek_opt_in_only, those scans can still skip live keys when
    range tombstones cross prefix boundaries, and unbounded legacy prefix-mode
    scans have other known holes. These stay documented limitations; only
    read-path conversion is turned off for them.
  • Using the prefix optimizations of plain prefix_same_as_start in the other
    layers when total_order_seek or auto_prefix_mode is also set is left for a
    follow-up.

Differential Revision: D129901533

@meta-cla meta-cla Bot added the CLA Signed label Nov 6, 2026
@meta-codesync

meta-codesync Bot commented Nov 6, 2026

Copy link
Copy Markdown

@xingbowang has exported this pull request. If you are a Meta employee, you can view the originating Diff in D129901533.

@github-actions

github-actions Bot commented Nov 6, 2026 •

Copy link
Copy Markdown

✅ clang-tidy: No findings on changed lines

Completed in 753.1s.

@meta-codesync meta-codesync Bot changed the title Fix prefix-mode SeekToFirst/SeekToLast skipping live keys Fix prefix_same_as_start iterator correctness gaps Nov 10, 2026
@meta-codesync meta-codesync Bot changed the title Fix prefix_same_as_start iterator correctness gaps Fix prefix_same_as_start iterator correctness gaps (#15606) Nov 11, 2026
xingbowang added a commit to xingbowang/rocksdb that referenced this pull request Nov 11, 2026
Summary:

Why the change is required:
The crash test found prefix_same_as_start iterators returning wrong keys in
two ways, both caused by child iterators using prefix filters (SST prefix
filter, memtable prefix bloom, LevelIterator prefix exhaustion) for a key in a
different prefix than the one the caller iterates:
1. A scan with no seek prefix to bound it: SeekToFirst() / SeekToLast() (with
   a bound they take the prefix from the bound), or Seek() / SeekForPrev()
   with a target outside the prefix extractor's domain. With a range
   tombstone that crosses a prefix boundary, MergingIterator reseeks older
   children to the tombstone boundary key, a prefix filter can drop a child,
   and the scan can skip live keys. Read-path tombstone conversion
   (min_tombstones_for_range_conversion, EXPERIMENTAL) trusts these scans, so
   a skipped live key could be covered by a converted tombstone and deleted
   for good. The same holds for implicit prefix seek (default ReadOptions).
   Also, SeekToLast() with iterate_upper_bound becomes
   SeekForPrev(upper_bound), which takes its prefix from an exclusive bound
   the iterator can never return: keys "abcd1".."abcd9" with upper bound
   "abce" return nothing.
2. With prefix_same_as_start and total_order_seek both set, a direction change
   (Next() after Prev()) can skip keys inside the seek prefix. Under
   DBOptions::prefix_seek_opt_in_only, total_order_seek is forced on, so every
   prefix_same_as_start iterator is exposed.

Previous behavior:
1. SeekToFirst()/SeekToLast() and out-of-domain seeks in prefix seek mode were
   never promised to match total order (the option docs call keys outside the
   seek prefix undefined, and the Prefix-Seek wiki says these calls are not
   well supported), but they ran, could return wrong results, and could
   create read-path range tombstones.
2. Since prefix_seek_opt_in_only was added (9.7.0), prefix_same_as_start and
   auto_prefix_mode enable prefix filtering even with total_order_seek=true,
   so for iterators total_order_seek only turns off the implicit prefix seek
   of default ReadOptions. This was stated in that release note, but not in
   options.h, where total_order_seek still read as "always total order".
   DBIter still treated total_order_seek=true as "the inner iterator is total
   order". So DBIter::ReverseToForward() skipped its reseek to the current
   key, and MergingIterator::SwitchToForward() reseeked the other children to
   the previous key, which can have another prefix. A child without that
   prefix was dropped by its prefix filter, losing its keys in the seek
   prefix.

New behavior:
1. With DBOptions::prefix_seek_opt_in_only, a prefix_same_as_start scan must
   take its prefix from an in-domain seek key. SeekToFirst() and SeekToLast(),
   with or without bounds, leave the iterator !Valid() with
   Status::NotSupported, and Seek()/SeekForPrev() with an out-of-domain target
   leave it !Valid() with Status::InvalidArgument. Callers Seek() or
   SeekForPrev() to a key in the wanted prefix instead.
   Without prefix_seek_opt_in_only, iteration results are unchanged, but an
   iterator in prefix seek mode (prefix_same_as_start or implicit prefix seek)
   no longer creates read-path range tombstones from a scan started by
   SeekToFirst() without iterate_lower_bound, SeekToLast() without
   iterate_upper_bound, or an out-of-domain seek target.
2. In DBIter, prefix_same_as_start takes precedence over total_order_seek and
   auto_prefix_mode, so DBIter reseeks on a direction change. A
   prefix_same_as_start iterator returns the same keys with or without
   total_order_seek, and so with or without prefix_seek_opt_in_only for
   Seek()-started scans.
Total order and auto_prefix_mode iteration results are not affected.

Implementation:
- GetIteratorPrefixMode() in options_helper.h decides how an iterator uses the
  prefix extractor (kTotalOrder, kAuto, kPrefix), with the order
  prefix_same_as_start > auto_prefix_mode > total_order_seek > implicit prefix
  seek. DBIter uses it for its total-order assumption; that is the
  direction-change fix. MemTable, PlainTable, the block-based table
  check_filter, LevelIterator files_filterable_on_seek_ and db_stress
  VerifyIterator use it where it gives the same result as before. The
  MergingIterator prefix flag, LevelIterator prefix exhaustion, the SST
  auto_prefix_mode upper bound check and the hash index keep their old
  conditions, so their behavior is unchanged.
- DBIter: under prefix_seek_opt_in_only, prefix_same_as_start SeekToFirst()
  and SeekToLast() return NotSupported before redirecting to a bound, and a
  new CheckSeekTargetPrefix() at the start of Seek()/SeekForPrev() returns
  InvalidArgument for an out-of-domain target. Otherwise DBIter remembers when
  the current scan uses prefix seek but has no prefix to bound it (unbounded
  SeekToFirst()/SeekToLast(), or out-of-domain target), and
  MaybeInsertRangeTombstone(), the only place that writes a converted
  tombstone, does nothing then.
- options.h: ReadOptions::total_order_seek now starts with its iterator
  meaning (turns off the implicit prefix seek; prefix_same_as_start and
  auto_prefix_mode take precedence; forced on by prefix_seek_opt_in_only).
  auto_prefix_mode notes that prefix_same_as_start takes precedence.
  prefix_seek_opt_in_only and prefix_same_as_start document the NotSupported /
  InvalidArgument cases and the scans that are not well supported.
  iterate_lower_bound allows a bound with another prefix under
  prefix_same_as_start.
- Release notes: a behavior change for NotSupported / InvalidArgument and the
  conversion change, and a bug fix for the direction change.
- db_stress: new prefix_seek_opt_in_only flag, randomized by db_crashtest.py.
  TestIterate sometimes sets total_order_seek together with
  prefix_same_as_start, and under prefix_seek_opt_in_only checks that
  SeekToFirst()/SeekToLast() return NotSupported. The prefix_same_as_start
  SeekToFirst() verification is removed: it checks a known limitation, and
  range tombstones left by earlier crash-test runs would make it fail. The
  bug it covered keeps its unit tests in file/prefetch_test.cc.

Safety, compatibility, and limitations:
- No API or format change. Users with prefix_seek_opt_in_only who call
  SeekToFirst()/SeekToLast() on a prefix_same_as_start iterator, or seek it to
  an out-of-domain key, now get NotSupported / InvalidArgument; they can
  Seek()/SeekForPrev() to a key in the wanted prefix, or use total order or
  auto_prefix_mode.
- prefix_same_as_start with total_order_seek or prefix_seek_opt_in_only now
  reseeks on a direction change, the cost plain prefix_same_as_start already
  pays.
- Without prefix_seek_opt_in_only, those scans can still skip live keys when
  range tombstones cross prefix boundaries, and unbounded legacy prefix-mode
  scans have other known holes. These stay documented limitations; only
  read-path conversion is turned off for them.
- Using the prefix optimizations of plain prefix_same_as_start in the other
  layers when total_order_seek or auto_prefix_mode is also set is left for a
  follow-up.

Differential Revision: D129901533
Summary:
Pull Request resolved: meta#15606

Why the change is required:
The crash test found prefix_same_as_start iterators returning wrong keys in
two ways, both caused by child iterators using prefix filters (SST prefix
filter, memtable prefix bloom, LevelIterator prefix exhaustion) for a key in a
different prefix than the one the caller iterates:
1. A scan with no seek prefix to bound it: SeekToFirst() / SeekToLast() (with
   a bound they take the prefix from the bound), or Seek() / SeekForPrev()
   with a target outside the prefix extractor's domain. With a range
   tombstone that crosses a prefix boundary, MergingIterator reseeks older
   children to the tombstone boundary key, a prefix filter can drop a child,
   and the scan can skip live keys. Read-path tombstone conversion
   (min_tombstones_for_range_conversion, EXPERIMENTAL) trusts these scans, so
   a skipped live key could be covered by a converted tombstone and deleted
   for good. The same holds for implicit prefix seek (default ReadOptions).
   Also, SeekToLast() with iterate_upper_bound becomes
   SeekForPrev(upper_bound), which takes its prefix from an exclusive bound
   the iterator can never return: keys "abcd1".."abcd9" with upper bound
   "abce" return nothing.
2. With prefix_same_as_start and total_order_seek both set, a direction change
   (Next() after Prev()) can skip keys inside the seek prefix. Under
   DBOptions::prefix_seek_opt_in_only, total_order_seek is forced on, so every
   prefix_same_as_start iterator is exposed.

Previous behavior:
1. SeekToFirst()/SeekToLast() and out-of-domain seeks in prefix seek mode were
   never promised to match total order (the option docs call keys outside the
   seek prefix undefined, and the Prefix-Seek wiki says these calls are not
   well supported), but they ran, could return wrong results, and could
   create read-path range tombstones.
2. Since prefix_seek_opt_in_only was added (9.7.0), prefix_same_as_start and
   auto_prefix_mode enable prefix filtering even with total_order_seek=true,
   so for iterators total_order_seek only turns off the implicit prefix seek
   of default ReadOptions. This was stated in that release note, but not in
   options.h, where total_order_seek still read as "always total order".
   DBIter still treated total_order_seek=true as "the inner iterator is total
   order". So DBIter::ReverseToForward() skipped its reseek to the current
   key, and MergingIterator::SwitchToForward() reseeked the other children to
   the previous key, which can have another prefix. A child without that
   prefix was dropped by its prefix filter, losing its keys in the seek
   prefix.

New behavior:
1. With DBOptions::prefix_seek_opt_in_only, a prefix_same_as_start scan must
   take its prefix from an in-domain seek key. SeekToFirst() and SeekToLast(),
   with or without bounds, leave the iterator !Valid() with
   Status::NotSupported, and Seek()/SeekForPrev() with an out-of-domain target
   leave it !Valid() with Status::InvalidArgument. Callers Seek() or
   SeekForPrev() to a key in the wanted prefix instead.
   Without prefix_seek_opt_in_only, iteration results are unchanged, but an
   iterator in prefix seek mode (prefix_same_as_start or implicit prefix seek)
   no longer creates read-path range tombstones from a scan started by
   SeekToFirst() without iterate_lower_bound, SeekToLast() without
   iterate_upper_bound, or an out-of-domain seek target.
2. In DBIter, prefix_same_as_start takes precedence over total_order_seek and
   auto_prefix_mode, so DBIter reseeks on a direction change. A
   prefix_same_as_start iterator returns the same keys with or without
   total_order_seek, and so with or without prefix_seek_opt_in_only for
   Seek()-started scans.
Total order and auto_prefix_mode iteration results are not affected.

Implementation:
- GetIteratorPrefixMode() in options_helper.h decides how an iterator uses the
  prefix extractor (kTotalOrder, kAuto, kPrefix), with the order
  prefix_same_as_start > auto_prefix_mode > total_order_seek > implicit prefix
  seek. DBIter uses it for its total-order assumption; that is the
  direction-change fix. MemTable, PlainTable, the block-based table
  check_filter, LevelIterator files_filterable_on_seek_ and db_stress
  VerifyIterator use it where it gives the same result as before. The
  MergingIterator prefix flag, LevelIterator prefix exhaustion, the SST
  auto_prefix_mode upper bound check and the hash index keep their old
  conditions, so their behavior is unchanged.
- DBIter: under prefix_seek_opt_in_only, prefix_same_as_start SeekToFirst()
  and SeekToLast() return NotSupported before redirecting to a bound, and a
  new CheckSeekTargetPrefix() at the start of Seek()/SeekForPrev() returns
  InvalidArgument for an out-of-domain target. Otherwise DBIter remembers when
  the current scan uses prefix seek but has no prefix to bound it (unbounded
  SeekToFirst()/SeekToLast(), or out-of-domain target), and
  MaybeInsertRangeTombstone(), the only place that writes a converted
  tombstone, does nothing then.
- options.h: ReadOptions::total_order_seek now starts with its iterator
  meaning (turns off the implicit prefix seek; prefix_same_as_start and
  auto_prefix_mode take precedence; forced on by prefix_seek_opt_in_only).
  auto_prefix_mode notes that prefix_same_as_start takes precedence.
  prefix_seek_opt_in_only and prefix_same_as_start document the NotSupported /
  InvalidArgument cases and the scans that are not well supported.
  iterate_lower_bound allows a bound with another prefix under
  prefix_same_as_start.
- Release notes: a behavior change for NotSupported / InvalidArgument and the
  conversion change, and a bug fix for the direction change.
- db_stress: new prefix_seek_opt_in_only flag, randomized by db_crashtest.py.
  TestIterate sometimes sets total_order_seek together with
  prefix_same_as_start, and under prefix_seek_opt_in_only checks that
  SeekToFirst()/SeekToLast() return NotSupported. The prefix_same_as_start
  SeekToFirst() verification is removed: it checks a known limitation, and
  range tombstones left by earlier crash-test runs would make it fail. The
  bug it covered keeps its unit tests in file/prefetch_test.cc.

Safety, compatibility, and limitations:
- No API or format change. Users with prefix_seek_opt_in_only who call
  SeekToFirst()/SeekToLast() on a prefix_same_as_start iterator, or seek it to
  an out-of-domain key, now get NotSupported / InvalidArgument; they can
  Seek()/SeekForPrev() to a key in the wanted prefix, or use total order or
  auto_prefix_mode.
- prefix_same_as_start with total_order_seek or prefix_seek_opt_in_only now
  reseeks on a direction change, the cost plain prefix_same_as_start already
  pays.
- Without prefix_seek_opt_in_only, those scans can still skip live keys when
  range tombstones cross prefix boundaries, and unbounded legacy prefix-mode
  scans have other known holes. These stay documented limitations; only
  read-path conversion is turned off for them.
- Using the prefix optimizations of plain prefix_same_as_start in the other
  layers when total_order_seek or auto_prefix_mode is also set is left for a
  follow-up.

Differential Revision: D129901533

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant